[sandbox audit] Address background processes by their server-assigned id - #4836
Draft
Wauplin wants to merge 1 commit into
Draft
[sandbox audit] Address background processes by their server-assigned id#4836Wauplin wants to merge 1 commit into
Wauplin wants to merge 1 commit into
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
`SandboxProcess.kill()` sent the OS pid. The server matches on the opaque id it assigned (`p-3`) and answered 200 regardless, so the call stopped nothing, reported success, and left the process running -- which also kept the sandbox non-idle, so idle eviction never fired and the job kept billing. `SandboxProcess` now carries `id` as its identifier and `kill()` uses it. `pid` stays, documented as observational: it is useful for correlating with `ps` inside the sandbox, but the OS may reuse it. `kill()` returns whether this call is what stopped the process, so a caller can tell "stopped it" from "it had already exited" -- both are successes, but they are different answers. A process from a host running a server that predates opaque ids has `id=None`; `kill()` raises with an explanation rather than sending a pid the new server will reject. The test fake is the reason this survived a green suite: it deleted by pid, so it implemented a protocol nobody serves. It now deletes by opaque id only and 400s on a pid, exactly like the real server -- and there is a test that asserts the pid path *fails*, so the fake going lax again would be caught rather than silently re-enabling the bug. Also corrects the `processes()` docstring: the server keeps a bounded number of finished processes, not all of them until the sandbox is deleted. Requires the server change to be deployed first. Validation: 60 tests pass, up from 55. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wauplin
force-pushed
the
security/opaque-process-ids
branch
from
September 8, 2026 15:11
87ba141 to
4b27a62
Compare
Wauplin
changed the base branch from
security/host-adoption-admission-control
to
security/pool-lifecycle-ownership
September 8, 2026 15:11
Wauplin
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
SandboxProcess.kill()sent the OS pid. The server matches on the opaque id it assigned(
p-3) and answered200regardless. So the call stopped nothing, reported success, and leftthe process running — which also kept the sandbox non-idle, so idle eviction never fired and
the job kept billing.
What changed
SandboxProcesscarriesidas its identifier;kill()uses it.pidstays, documented as observational — useful for correlating withpsinside thesandbox, but the OS may reuse it.
kill()returns whether this call stopped the process, so a caller can distinguish"stopped it" from "it had already exited". Both are successes; they're different answers.
id=None;kill()raises with anexplanation rather than sending a pid the new server will reject.
processes()docstring corrected: the server keeps a bounded number of finishedprocesses, not all of them until the sandbox is deleted.
The fake server was the root cause
This survived 41 green tests because the fake deleted by pid — it implemented a protocol
nobody serves. It now deletes by opaque id only and 400s on a pid, exactly like the real
server. And there's a test that asserts the pid path fails, so if the fake goes lax again
that gets caught rather than silently re-enabling the bug.
This is the second instance of the same failure mode in this stack (the other was the fake
accepting the host token on every route), which is why it's worth fixing the fake rather than
just the client.
Validation
60 tests pass, up from 55. New coverage:
kill()sends the opaque id; it's idempotent andreports which case; listed processes carry their id; sending a pid is refused; a process
without an id refuses to be killed with a clear message.
Behaviour changes
kill()returnsboolinstead ofNone. Existing code ignoring the return value isunaffected.
kill()now raises instead of silently doing nothing. That is thepoint, but it means pool hosts booted before the server upgrade need recycling for
kill()to work at all — previously it "worked" by doing nothing.Note
Medium Risk
Behavior change: kill() now raises on old servers without opaque ids (pool hosts need recycling after server deploy); return type changed from None to bool.
Overview
Fixes
SandboxProcess.kill()so it actually terminates background work: it now sendsDELETE /processes/{id}with the server’s opaque process id (e.g.p-0), not the OS pid the server ignores while still returning success.SandboxProcessgains anidfield (from spawn/list responses);pidstays for correlating withpsonly.kill()returnsbool(Trueif this call stopped it,Falseif already gone) and raisesSandboxErrorwhenidis missing (pre-upgrade pool hosts)—instead of silently doing nothing.Docs drop the “kill doesn’t work” caveat and clarify that finished processes are listed only up to a bounded server history. Tests align the fake server with production (kill by id, 400 on pid) and add regression coverage for idempotent kill and legacy hosts.
Reviewed by Cursor Bugbot for commit 4b27a62. Bugbot is set up for automated code reviews on this repo. Configure here.